cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087

mauriciovasquezbernal · 2019-07-25T15:31:42Z

Currently, the parent process of the container is moved to the right cgroup-v2 tree when systemd is using a hybrid model (last line with 0::):

$ runc --systemd-cgroup run myid
/ # cat /proc/self/cgroup
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/system.slice/runc-myid.scope

However, if a second process is executed in the same container, it is not moved to the right cgroup-v2 tree:

$ runc exec myid /bin/sh -c 'cat /proc/self/cgroup'
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/user.slice/user-1000.slice/session-8.scope

Having the processes of the container in its own cgroup-v2 is useful for any BPF programs that rely on bpf_get_current_cgroup_id(), like https://github.com/kinvolk/inspektor-gadget/ for instance.

This commit makes that processes executed with exec are placed into the right cgroup-v2 tree. The implementation checks if systemd is using a hybrid mode (by checking if cgroups-v2 is mounted in /sys/fs/cgroup/unified), if yes, the path of the cgroup-v2 slice for this container is saved into the cgroup path list.

libcontainer/cgroups/systemd/apply_systemd.go

Fix an issue of cgroup v2 path being incorrectly configured when using the hybrid mode in systemd. Bump docker-runc to 1.0.0-rc8-r2 as well. See also opencontainers/runc#2087

mauriciovasquezbernal · 2019-08-26T13:18:40Z

Hello folks, any thoughts about this one?

Thanks!

/cc @filbranden, @crosbymichael @giuseppe @cyphar

dongsupark · 2019-09-21T11:50:58Z

@cyphar
It's what I mentioned during the talk in the hallway.

libcontainer/cgroups/systemd/apply_systemd.go

mauriciovasquezbernal · 2020-03-10T12:41:14Z

@AkihiroSuda I changed the names according to your suggestion.

AkihiroSuda · 2020-03-10T13:01:03Z

Please squash commits and sign

libcontainer/cgroups/systemd/apply_systemd.go

AkihiroSuda · 2020-03-10T16:04:24Z

LGTM

AkihiroSuda · 2020-03-10T16:04:45Z

@opencontainers/runc-maintainers PTAL

AkihiroSuda · 2020-03-30T21:58:35Z

@mrunalp @kolyshkin PTAL

kolyshkin · 2020-06-23T01:38:29Z

Should we ever try to support hybrid mode? If yes, it should at least be tested. Currently, we barely test v2.

dongsupark · 2020-07-09T09:30:26Z

FYI, I have just realized that this PR needs a rebase.
Apparently code around cgroups/systemd has changed recently with v1.0.0-rc91.

kolyshkin · 2021-02-03T03:27:34Z

I think we don't want to support this two-headed beast of hybrid mode, and recommend using cgroup v2 unified instead. IOW close this.

@opencontainers/runc-maintainers WDYT?

cyphar · 2021-02-03T04:22:08Z

Yeah supporting hybrid mode will only lead to madness. cgroupv2 now supports all of the major controllers so hybrid mode doesn't really make sense anymore. I think we should close this.

AkihiroSuda · 2021-02-03T05:18:40Z

I agree that hybrid mode is meaningless for controllers like cpu and memory, but it is still useful for eBPF stuff.
So I think it wouldn't hurt to merge this if somebody can rebase this.

Anyway, we will need to close this if this remains unrebased.

mauriciovasquezbernal · 2021-02-05T20:56:35Z

@AkihiroSuda @kolyshkin @cyphar I rebased this.

cyphar · 2021-02-07T05:20:25Z

@kolyshkin Does this change make sense given all of the changes you made last year to how subsystem paths are handled in cgroupfs?

kolyshkin · 2021-02-10T01:07:20Z

libcontainer/cgroups/v1_utils.go

@@ -116,6 +116,12 @@ func FindCgroupMountpoint(cgroupPath, subsystem string) (string, error) {
 		return "", errUnified
 	}

+	// If subsystem is empty it means that we are looking for the
+	// cgroups2 path


I'd say "cgroup2 hybrid path" instead.

kolyshkin · 2021-02-10T01:17:23Z

Looks like it still makes sense. Two things

fs cgroup driver is not fixed wrt hybrid mode;
I don't like the hack of using empty subsystem to denote the hybrid path much. Perhaps it can be better implemented in fs driver by adding NameGroup{Name: "hybrid", Join: yes"} to the list of subsystems, and modifying other places to support a special value of "hybrid".

Finally, if we're doing it, we need a test case (apparently ubuntu 20.04 which is used on GHA is a good candidate for that).

kolyshkin · 2021-02-10T03:39:32Z

(CI failure on centos7 is unrelated; it's #2760)

AkihiroSuda · 2021-02-10T04:01:47Z

I don't like the hack of using empty subsystem to denote the hybrid path much. Perhaps it can be better implemented in fs driver by adding NameGroup{Name: "hybrid", Join: yes"} to the list of subsystems, and modifying other places to support a special value of "hybrid".

The empty string corresponds to /proc/PID/cgroup . Also, v1 named group has nothing to do with the hybrid hierarchy.

kolyshkin · 2021-02-11T21:05:14Z

The empty string corresponds to /proc/PID/cgroup

Yes, I understand that it comes from there.

Also, v1 named group has nothing to do with the hybrid hierarchy.

... and yet we put the path to it into a map of paths for v1 controllers.

Anyway, I'm fine with the current approach -- it's a hack but at least it is documented in the code.

The other two issues remain:

fs cgroup driver is not fixed wrt hybrid mode;
need a simple test case.

Currently the parent process of the container is moved to the right cgroup v2 tree when systemd is using a hybrid model (last line with 0::): $ runc --systemd-cgroup run myid / # cat /proc/self/cgroup 12:cpuset:/system.slice/runc-myid.scope 11:blkio:/system.slice/runc-myid.scope 10:devices:/system.slice/runc-myid.scope 9:hugetlb:/system.slice/runc-myid.scope 8:memory:/system.slice/runc-myid.scope 7:rdma:/ 6:perf_event:/system.slice/runc-myid.scope 5:net_cls,net_prio:/system.slice/runc-myid.scope 4:freezer:/system.slice/runc-myid.scope 3:pids:/system.slice/runc-myid.scope 2:cpu,cpuacct:/system.slice/runc-myid.scope 1:name=systemd:/system.slice/runc-myid.scope 0::/system.slice/runc-myid.scope However, if a second process is executed in the same container, it is not moved to the right cgroup v2 tree: $ runc exec myid /bin/sh -c 'cat /proc/self/cgroup' 12:cpuset:/system.slice/runc-myid.scope 11:blkio:/system.slice/runc-myid.scope 10:devices:/system.slice/runc-myid.scope 9:hugetlb:/system.slice/runc-myid.scope 8:memory:/system.slice/runc-myid.scope 7:rdma:/ 6:perf_event:/system.slice/runc-myid.scope 5:net_cls,net_prio:/system.slice/runc-myid.scope 4:freezer:/system.slice/runc-myid.scope 3:pids:/system.slice/runc-myid.scope 2:cpu,cpuacct:/system.slice/runc-myid.scope 1:name=systemd:/system.slice/runc-myid.scope 0::/user.slice/user-1000.slice/session-8.scope This commit makes that processes executed with exec are placed into the right cgroup v2 tree. The implementation checks if systemd is using a hybrid mode (by checking if cgroups v2 is mounted in /sys/fs/cgroup/unified), if yes, the path of the cgroup v2 slice for this container is saved into the cgroup path list. The fs group driver has a similar issue, in this case none of the runc run or runc exec commands put the process in the right cgroups v2. This commit also fixes that. Having the processes of the container in its own cgroup v2 is useful for any BPF programs that rely on bpf_get_current_cgroup_id(), like https://github.com/kinvolk/inspektor-gadget/ for instance. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

Check that runc run and runc exec put the process on the same cgroups v2 when using hybrid mode. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal · 2021-02-26T21:01:00Z

@kolyshkin I added a test and also implemented for the fs controller. I agree that using an empty string is not the cleanest solution, I tried to implement it in another way but I hit some issues. Unfortunately I don't have that much time to continue investigating other ways to implement this.

kolyshkin · 2021-08-31T17:26:23Z

@mauriciovasquezbernal I am carrying this one in #3059 but this one can still be merged first (once rebased).

Note that the last patch needs to put cgroup_hybrid before cgroup_* (added by commit 44fcbfd).

kolyshkin · 2021-08-31T17:27:54Z

@mauriciovasquezbernal I am carrying this one in #3059 but this one can still be merged first (once rebased).

Well, I'm actually wrong. This needs criu-3.16 which is still not released.

mauriciovasquezbernal · 2021-10-07T12:13:30Z

It was a very long one! Thanks a lot for driving it forward @kolyshkin!

crosbymichael reviewed Jul 25, 2019

View reviewed changes

libcontainer/cgroups/systemd/apply_systemd.go Outdated Show resolved Hide resolved

mauriciovasquezbernal mentioned this pull request Jul 30, 2019

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode kinvolk/runc#1

Closed

dongsupark mentioned this pull request Aug 2, 2019

app-emulation/docker-runc: fix cgroup v2 path for systemd hybrid mode flatcar-archive/coreos-overlay#58

Merged

AkihiroSuda added the kind/bug label Mar 9, 2020

AkihiroSuda reviewed Mar 9, 2020

View reviewed changes

libcontainer/cgroups/systemd/apply_systemd.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Mar 9, 2020

View reviewed changes

libcontainer/cgroups/systemd/apply_systemd.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Mar 10, 2020

View reviewed changes

libcontainer/cgroups/systemd/apply_systemd.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/pr/add_cgroupv2_path branch from 1485233 to c33ba15 Compare March 10, 2020 14:47

mauriciovasquezbernal mentioned this pull request Jun 16, 2020

Filtering by cgroups doesn't work iovisor/bcc#2971

Closed

This was referenced Jul 9, 2020

Upgrade Runc in Alpha from 1.0.0_rc90 to 1.0.0_rc91 flatcar-archive/coreos-overlay#463

Closed

[WIP] Upgrade Runc in Edge from 1.0.0_rc90 to 1.0.0_rc91 flatcar-archive/coreos-overlay#464

Closed

AkihiroSuda added the status/needs-rebase label Feb 3, 2021

mauriciovasquezbernal force-pushed the mauricio/pr/add_cgroupv2_path branch 3 times, most recently from cecb69d to b9df855 Compare February 5, 2021 20:30

AkihiroSuda removed the status/needs-rebase label Feb 6, 2021

cyphar requested a review from kolyshkin February 7, 2021 05:19

kolyshkin reviewed Feb 10, 2021

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/pr/add_cgroupv2_path branch from b9df855 to bfd64a7 Compare February 17, 2021 20:57

tests: add integration test for cgroups hybrid

c64eb7f

Check that runc run and runc exec put the process on the same cgroups v2 when using hybrid mode. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal force-pushed the mauricio/pr/add_cgroupv2_path branch from bfd64a7 to c64eb7f Compare February 17, 2021 21:14

This was referenced Jul 22, 2021

runc exec --cgroup #3059

Merged

[WIP] [carry #2087] cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #3107

Closed

kolyshkin added status/needs-rebase area/cgroupv2 labels Aug 31, 2021

kolyshkin mentioned this pull request Sep 3, 2021

Processes launched via docker exec are not placed into the correct cgroup moby/moby#42704

Closed

kolyshkin mentioned this pull request Sep 13, 2021

cgroups: Add support for cgroup.kill added in Linux Kernel 5.14 #3199

Closed

kolyshkin closed this in #3059 Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087

mauriciovasquezbernal commented Jul 25, 2019

mauriciovasquezbernal commented Aug 26, 2019

dongsupark commented Sep 21, 2019

mauriciovasquezbernal commented Mar 10, 2020

AkihiroSuda commented Mar 10, 2020

AkihiroSuda commented Mar 10, 2020 •

edited

Loading

AkihiroSuda commented Mar 10, 2020

AkihiroSuda commented Mar 30, 2020

kolyshkin commented Jun 23, 2020

dongsupark commented Jul 9, 2020

kolyshkin commented Feb 3, 2021

cyphar commented Feb 3, 2021 •

edited

Loading

AkihiroSuda commented Feb 3, 2021

mauriciovasquezbernal commented Feb 5, 2021

cyphar commented Feb 7, 2021

kolyshkin Feb 10, 2021

kolyshkin commented Feb 10, 2021

kolyshkin commented Feb 10, 2021

AkihiroSuda commented Feb 10, 2021

kolyshkin commented Feb 11, 2021

mauriciovasquezbernal commented Feb 26, 2021

kolyshkin commented Aug 31, 2021

kolyshkin commented Aug 31, 2021

mauriciovasquezbernal commented Oct 7, 2021

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087

Conversation

mauriciovasquezbernal commented Jul 25, 2019

mauriciovasquezbernal commented Aug 26, 2019

dongsupark commented Sep 21, 2019

mauriciovasquezbernal commented Mar 10, 2020

AkihiroSuda commented Mar 10, 2020

AkihiroSuda commented Mar 10, 2020 • edited Loading

AkihiroSuda commented Mar 10, 2020

AkihiroSuda commented Mar 30, 2020

kolyshkin commented Jun 23, 2020

dongsupark commented Jul 9, 2020

kolyshkin commented Feb 3, 2021

cyphar commented Feb 3, 2021 • edited Loading

AkihiroSuda commented Feb 3, 2021

mauriciovasquezbernal commented Feb 5, 2021

cyphar commented Feb 7, 2021

kolyshkin Feb 10, 2021

Choose a reason for hiding this comment

kolyshkin commented Feb 10, 2021

kolyshkin commented Feb 10, 2021

AkihiroSuda commented Feb 10, 2021

kolyshkin commented Feb 11, 2021

mauriciovasquezbernal commented Feb 26, 2021

kolyshkin commented Aug 31, 2021

kolyshkin commented Aug 31, 2021

mauriciovasquezbernal commented Oct 7, 2021

AkihiroSuda commented Mar 10, 2020 •

edited

Loading

cyphar commented Feb 3, 2021 •

edited

Loading